Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: align CIDR check rules with their title #307

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

nikpivkin
Copy link
Contributor

@nikpivkin nikpivkin commented Dec 24, 2024

Related issues:

Checks that require restricting access to or from any IP address now check only this condition. In addition, the titles, descriptions, and messages for all such checks have been unified.

@itaysk
Copy link
Contributor

itaysk commented Jan 2, 2025

@tamirkiviti13 do you have any feedback about this?

@tamirkiviti13
Copy link

@itaysk
LGTM, thanks

@nikpivkin nikpivkin marked this pull request as ready for review January 15, 2025 18:29
@nikpivkin nikpivkin requested a review from simar7 as a code owner January 15, 2025 18:29
Copy link
Contributor

@itaysk itaysk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. if we change the meaning of some check, should we also change the "short name" etc?
  2. do we no longer need the CIDR functions in Go?
  3. since we are changing the meaning of some checks, I wonder if we should revisit the severity too.
  4. additional discussion about check for unrestricted vs public in the original discussion: fix(misconf): improve CIDR related checks trivy#8184 (comment)

@nikpivkin
Copy link
Contributor Author

  1. It seems the new title should not allow unrestricted ... from the public internet may be confusing and should be changed to should not ... from any IP address. This PR does not change the meaning of the checks but aligns their logic with the title and description.
  2. Some checks still use Go functions.
  3. Indeed, the severity levels for the checks vary. I found the following levels used: Medium, High and Critical.

@simar7
Copy link
Member

simar7 commented Jan 16, 2025

if we change the meaning of some check, should we also change the "short name" etc?

short names, or any identifier for that matter should be kept unique and unchanged to ensure we don't cause any unexpected breakage. We would have to deprecate this and introduce a new check if change of names is desired.

# title: Network ACLs should not allow ingress from 0.0.0.0/0 to port 22 or port 3389.
# title: Network ACLs should not allow ingress from the public internet to port 22 or port 3389.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think /0 is much more straight forward to understand technically, than the public internet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you think of “any IP address”?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure that could work.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe "should not allow unrestricted ingress" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated 518ae9c

@nikpivkin nikpivkin changed the title fix: improve CIDR related checks fix: align CIDR check rules with their title Jan 16, 2025
@nikpivkin nikpivkin requested review from itaysk and simar7 January 16, 2025 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants